Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Improve Application Security #1889

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

feat: Improve Application Security #1889

wants to merge 17 commits into from

Conversation

adintegra
Copy link
Contributor

@adintegra adintegra commented Nov 18, 2024 •
edited
Loading

Tightening up security of the application in general:

  • Implement CSP
  • Better sanitisation of GraphQL queries to ensure only permitted datasources are used (addresses BAFU vulnerability report)

@vercel Vercel
Copy link

vercel bot commented Nov 18, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 1:48pm

@adintegra adintegra changed the title feat: Adds basic CSP configuration feat: Adds Content Security Policy Nov 18, 2024
@adintegra adintegra linked an issue Nov 19, 2024 that may be closed by this pull request
Vulnerability Report (SLA) #1806
Open
@adintegra adintegra requested a review from wereHamster December 3, 2024 12:28
@adintegra
Copy link
Contributor Author

@wereHamster, making these changes primarily due to the BAFU's report, but also generally, to improve overall security. Do you think the added headers make sense?

@adintegra adintegra removed the request for review from wereHamster December 5, 2024 14:40
@adintegra adintegra changed the title feat: Adds Content Security Policy feat: Improve Application Security Dec 5, 2024
@adintegra adintegra requested a review from ptbrowne December 6, 2024 14:18
@ptbrowne
Copy link
Collaborator

ptbrowne commented Dec 7, 2024 •
edited
Loading

I added validation of the data source url so that we could only used urls that are whitelisted in datasourceUrl.

I tried with https://www.npmjs.com/package/graphql-constraint-directive but could not make it work and it seems the directive only worked if the field was nested inside an object, which would have required a refactor of places using datasourceUrl.

Instead, I used a custom scalar, which also can be used for providing additional validation rules on existing scalars (see https://www.apollographql.com/docs/apollo-server/schema/custom-scalars).

@adintegra
Copy link
Contributor Author

Thanks @ptbrowne! Looks like this would be a good solution 👍🏼

Did you have issues with graphql-constraint-directive because of the version maybe? I initially missed the note about having to use v2 with graphql v15.

@ptbrowne
Copy link
Collaborator

ptbrowne commented Dec 7, 2024

I do not know, I did not see this note either, could be. Did it work for you ?

@ptbrowne
Merge branch 'main' into feat/implement-csp
15ab20c 
# Conflicts:
#	app/graphql/query-hooks.ts
#	app/graphql/resolver-types.ts
#	yarn.lock
@ptbrowne
feat: Move datasource to allowed
f3495cd
@adintegra adintegra self-assigned this Dec 19, 2024
@adintegra adintegra marked this pull request as ready for review December 19, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptbrowne ptbrowne Awaiting requested review from ptbrowne

@bprusinowski bprusinowski Awaiting requested review from bprusinowski bprusinowski is a code owner

@noahonyejese noahonyejese Awaiting requested review from noahonyejese noahonyejese is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@adintegra adintegra

Labels
development
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vulnerability Report (SLA)
2 participants
@adintegra @ptbrowne